Storage updates #5698
Storage updates #5698
Conversation
- Delete from storage when a project or version is deleted - Project delete now handled in a method on the model - Define media type constants (next step: use them everywhere) - A few places still using default storage instead of RTD_BUILD_MEDIA_STORAGE
|
One thing I didn't do in this PR that needs to be done before we turn off all storage on the local webs is to make sure that the code that handles |
| storage_paths = [] | ||
| for type_ in MEDIA_TYPES: | ||
| storage_paths.append( | ||
| '{}/{}'.format( |
There was a problem hiding this comment.
I think it's better to use either os.path.join or pathlib.Path here.
There was a problem hiding this comment.
This is not a path on the local disk. This is a storage path which is different. It always uses forward slashes regardless of platform.
There was a problem hiding this comment.
If we want to go fancy, what we need here is a PurePosixPath: it's platform independent and does not access the filesystem when manipulating paths.
There was a problem hiding this comment.
The Django storage API does not implement a posix standard either. I believe that using pathlib is not the correct course of action because these are not filesystem paths.
| ) | ||
| return storage.exists(storage_path) | ||
|
|
||
| return False | ||
|
|
||
| def has_htmlzip(self, version_slug=LATEST): |
There was a problem hiding this comment.
nit: these 3 methods are exactly the same; the only change is type_ value. They could be refactored to use the same chunk of code.
Not needed to be done in this PR, though.
There was a problem hiding this comment.
That seems like a worthwhile refactor and it seems useful as part of this PR.
| @@ -1680,6 +1680,20 @@ def remove_dirs(paths): | |||
| shutil.rmtree(path, ignore_errors=True) | |||
|
|
|||
|
|
|||
| @app.task() | |||
| def remove_build_storage_paths(paths): | |||
There was a problem hiding this comment.
I'm not sure what we decided here, but I think that build servers don't have the credentials (or the permissions) to make this operation. I think this task may be only executed by webs. In that case, queue='web' should be passed in the decorator.
There was a problem hiding this comment.
It should only be executed by webs typically in response to a project or version being deleted completely. I'll update it.
There was a problem hiding this comment.
I think you forgot to push this change.
|
I would like to find a better way to test this. It can be a little tricky because many of the functions check both the local disk and storage which in test should probably be the same thing. With that said, there still might be a better approach to testing. |
Since #5549 was merged (although it is not yet deployed) a few issues emerged. This is an attempt to fix them:
Project.delete())RTD_BUILD_MEDIA_STORAGE